Skip to content

Builder.withoutExemplars() setting cannot be overridden via properties#1477

Merged
zeitlinger merged 1 commit intoprometheus:mainfrom
cruftex:exemplars_off
Aug 6, 2025
Merged

Builder.withoutExemplars() setting cannot be overridden via properties#1477
zeitlinger merged 1 commit intoprometheus:mainfrom
cruftex:exemplars_off

Conversation

@cruftex
Copy link
Contributor

@cruftex cruftex commented Jul 23, 2025

The PR will fix the behaviour that exemplar support can be switched on via the properties, although it was disabled explicitly in the code definition by withoutExemplars. Currently this would enable exemplars:

    PrometheusProperties properties = PrometheusProperties.builder()
      .putMetricProperty("count", MetricsProperties.builder()
        .exemplarsEnabled(true)
        .build())
      .build();
    Counter counter =
      Counter.builder(properties)
        .name("count_total")
        .withoutExemplars()
        .build();

In TDD fashion I will add failing tests first.

@cruftex cruftex mentioned this pull request Jul 23, 2025
Open
@cruftex cruftex force-pushed the exemplars_off branch 3 times, most recently from 3444294 to 3e792c9 Compare July 23, 2025 13:06
@cruftex cruftex changed the title Work in progress: withoutExemplars cannot be overridden via specific metric property Builder.withoutExemplars() setting cannot be overridden via properties Jul 23, 2025
@cruftex
Copy link
Contributor Author

cruftex commented Jul 23, 2025

@zeitlinger
This PR is now ready for review and merging.

Somehow the build is not starting. Is there something else I need to do?

@zeitlinger
Copy link
Member

zeitlinger commented Jul 24, 2025

Somehow the build is not starting. Is there something else I need to do?

no - that needs to be approved

@zeitlinger zeitlinger enabled auto-merge (squash) July 24, 2025 07:35
@zeitlinger
Copy link
Member

zeitlinger commented Jul 24, 2025 •
edited
Loading

@cruftex the code is not formatted correctly - run ./mvnw spotless:apply

auto-merge was automatically disabled July 24, 2025 09:35

Head branch was pushed to by a user without write access

@cruftex
Copy link
Contributor Author

cruftex commented Jul 24, 2025

@zeitlinger sorry... done!

@zeitlinger zeitlinger enabled auto-merge (squash) July 24, 2025 11:56
@zeitlinger
Copy link
Member

zeitlinger commented Jul 24, 2025

now there's a checkstyle issue

[WARN] /home/runner/work/client_java/client_java/prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Summary.java:52:5: Distance between variable 'exemplarsEnabled' declaration and its first usage is 4, but allowed 3. Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
[WARN] /home/runner/work/client_java/client_java/prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Histogram.java:118:5: Distance between variable 'exemplarsEnabled' declaration and its first usage is 7, but allowed 3. Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
Audit done.

@cruftex
Make sure when Builder.withoutExemplars is used, that this cannot be …
b6eba2e 
…overridden via properties. Introduce builder for PrometheusProperties for testing. Minimal housekeeping, removing the redundant boolean flag.

Signed-off-by: Jens Wilke <signed-off@cruftex.net>
auto-merge was automatically disabled July 24, 2025 16:21

Head branch was pushed to by a user without write access

@cruftex
Copy link
Contributor Author

cruftex commented Jul 24, 2025

I agree with checkstyle. Corrected.

@cruftex
Copy link
Contributor Author

cruftex commented Jul 29, 2025

@zeitlinger ping

@zeitlinger zeitlinger enabled auto-merge (squash) August 6, 2025 11:11
@zeitlinger zeitlinger merged commit f971724 into prometheus:main Aug 6, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zeitlinger zeitlinger zeitlinger approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cruftex @zeitlinger